Cache for git remote - #1882
Conversation
There was a problem hiding this comment.
2 issues found across 9 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
f5279b8 to
cf7ad82
Compare
|
I think #1836 already covers the component.* side nicely, so I'm going to close this one to keep things simple. |
|
Well @firecow . Here is one of my Without this patch, it takes 19 seconds to run With it, 3 seconds. This is with the cache already there for both. Multiple component at the same version from the same catalog is the reason this takes a long time without this optimization. |
|
Holy christ 😄 Ok, I'm reopening. Jesus, what a pipeline. |
5652d2b to
1c3a1fd
Compare
| promises.push((async (componentParseCache: Map<number, ParsedComponent>, component: ParsedComponent, opts: ParserIncludesInitOptions) => { | ||
| if (!component.isLocal) { | ||
| await this.downloadIncludeComponent(opts, component.projectPath, component.effectiveRef, component.componentPath); | ||
| } | ||
| componentParseCache.set(index, component); | ||
| })(componentParseCache, component, opts)); | ||
| } |
There was a problem hiding this comment.
| promises.push((async (componentParseCache: Map<number, ParsedComponent>, component: ParsedComponent, opts: ParserIncludesInitOptions) => { | |
| if (!component.isLocal) { | |
| await this.downloadIncludeComponent(opts, component.projectPath, component.effectiveRef, component.componentPath); | |
| } | |
| componentParseCache.set(index, component); | |
| })(componentParseCache, component, opts)); | |
| } | |
| promises.push((async () => { | |
| if (!component.isLocal) { | |
| await this.downloadIncludeComponent(opts, component.projectPath, component.effectiveRef, component.componentPath); | |
| } | |
| componentParseCache.set(index, component); | |
| })()); |
| const cmdStr = cmdArgs.join(" "); | ||
| let info = this.gitRemoteInfoCache[cmdStr]; | ||
| if (!(cmdStr in this.gitRemoteInfoCache)) { | ||
| info = Utils.syncSpawn(cmdArgs).stdout; | ||
| this.gitRemoteInfoCache[cmdStr] = info; | ||
| } | ||
| return info; | ||
| }; |
There was a problem hiding this comment.
| const cmdStr = cmdArgs.join(" "); | |
| let info = this.gitRemoteInfoCache[cmdStr]; | |
| if (!(cmdStr in this.gitRemoteInfoCache)) { | |
| info = Utils.syncSpawn(cmdArgs).stdout; | |
| this.gitRemoteInfoCache[cmdStr] = info; | |
| } | |
| return info; | |
| }; | |
| const cmdStr = cmdArgs.join(" "); | |
| if (!(cmdStr in this.gitRemoteInfoCache)) { | |
| this.gitRemoteInfoCache[cmdStr] = Utils.syncSpawn(cmdArgs).stdout; | |
| } | |
| return this.gitRemoteInfoCache[cmdStr]; | |
| } |
| if (gitRemoteMatch?.groups == null) throw new Error(`This is a bug, please create a github issue if this is something you're expecting to work. input: ${component}`); | ||
| const {domain, projectPath, port, componentName, ref} = gitRemoteMatch.groups; | ||
| const isLocalComponent = projectPath === `${gitData.remote.group}/${gitData.remote.project}` && ref === gitData.commit.SHA; | ||
| const parserIncludes = this; // eslint-disable-line @typescript-eslint/no-this-alias |
There was a problem hiding this comment.
| const parserIncludes = this; // eslint-disable-line @typescript-eslint/no-this-alias |
| if (this.reference == "~latest" || semanticVersionRangesPattern.test(this.reference)) { | ||
| // https://docs.gitlab.com/ci/components/#semantic-version-ranges | ||
| const stdout = getGitRemoteInfo(this, "--tags"); | ||
| const stdout = parserIncludes.getGitRemoteInfo(this, "--tags"); |
There was a problem hiding this comment.
| const stdout = parserIncludes.getGitRemoteInfo(this, "--tags"); | |
| const stdout = ParserIncludes.getGitRemoteInfo(this, "--tags"); |
| this._cache.sha = this.effectiveRef; | ||
| } else { | ||
| const stdout = getGitRemoteInfo(this); | ||
| const stdout = parserIncludes.getGitRemoteInfo(this); |
There was a problem hiding this comment.
| const stdout = parserIncludes.getGitRemoteInfo(this); | |
| const stdout = ParserIncludes.getGitRemoteInfo(this); |
firecow
left a comment
There was a problem hiding this comment.
The 5 suggestions must be batched. Deleting the alias line alone breaks the build; lines 300 and 326 still reference parserIncludes.
Also: the diff still contains the component.* work #1836 superseded; the rebase you asked for should reduce it to just the caching change.
|
|
||
| export class ParserIncludes { | ||
| private static count: number = 0; | ||
| private static gitRemoteInfoCache: Record<string, string> = {}; |
There was a problem hiding this comment.
Never cleared, though resetCount() sits right beside it for count. Stale git ls-remote results could leak between runs in the test suite. Broke nothing I ran, so your call whether it matters for a single-shot CLI.
There was a problem hiding this comment.
Yes I can reinit the remote info cache in the resetCount method. That would be good enough I think.
1c3a1fd to
eeea380
Compare
|
Should be good now. |
Based on #1836 it allows to keep some cache for remote refs so it’s a bit faster.
Summary by cubic
Speeds up include processing by caching identical
git ls-remotecalls and preventing races by writing the component parse cache only after remote downloads finish.Performance
git ls-remoteresults keyed by the full command; cleared viaParserIncludes.resetCount().componentParseCache.setuntil after remote component download completes to avoid parallel resolution races.Refactors
getGitRemoteInfowithParserIncludes.getGitRemoteInfoand update call sites.Written for commit eeea380. Summary will update on new commits.